Glasgow | 25-SDC-NOV | Katarzyna Kazimierczuk | Sprint 4 | Implement Shell Tools#324
Glasgow | 25-SDC-NOV | Katarzyna Kazimierczuk | Sprint 4 | Implement Shell Tools#324katarzynakaz wants to merge 1 commit intoCodeYourFuture:mainfrom
Conversation
katarzynakaz
added
📅 Sprint 3
katarzynakaz
changed the title
katarzynakaz
added
📅 Sprint 4
SlideGauge
added
the
Review in progress
SlideGauge
left a comment
SlideGauge
left a comment
There was a problem hiding this comment.
General note: I notced async/await being used here, I have doubts in necessity of it in this context: we're doing a console utility
|
|
||
| const argv = process.argv.slice(2); | ||
|
|
||
| switch (argv[0]) { |
There was a problem hiding this comment.
What is here is "pattern matching" of the cases from the task. Whilst it work for these cases, this is unfortunately not the way how production software is written.
Several moments:
- functions are referring to some "cases". It is not clear what they mean. Instead, function naming should reflect what the function does: output lines, output lines with numbering etc
- Hint: the code in all three functions has a lot of similarities: we read list of files, traverse the list one by one, apply some rule to the line (or no rule at all) and then output the line to console. Can we generalise this somehow, like have a function whose responsibility is to traverse the input list, then the function which reads the file and applies the rules? Try it. (hint: maybe usage of functions as objects passed into initial function may help)
| @@ -0,0 +1,46 @@ | |||
| import process from "node:process"; | |||
| import { promises as fs } from "node:fs"; | |||
| // const fs = promises | |||
|
|
||
|
|
||
| //one for 1 and 3 | ||
| async function printOneOrMore(listOfFiles) { |
There was a problem hiding this comment.
Nice that all functions are generalised to work with a list, instead (one function for single file, another function is for a list)
| // 2 * `cat -n sample-files/1.txt` | ||
| // 4 * `cat -n sample-files/*.txt` | ||
|
|
||
| async function caseTwoAndFour(listOfFiles) { |
There was a problem hiding this comment.
Overall, we do not need async/await pattern in the cat-like utility. We just sequentially read file line by line and at the same time the utility is a console utility
| } else if (argv.includes('sample-files')) { | ||
| await showVisibleInSampleFiles(); | ||
| } else { | ||
| await showCurrentDir(); |
There was a problem hiding this comment.
There is no such a function in this file
| // 4 * `cat -n sample-files/*.txt` | ||
|
|
||
| async function caseTwoAndFour(listOfFiles) { | ||
| for (const file of listOfFiles) { |
There was a problem hiding this comment.
cat -n with multiple files should continue line numbering across files — i.e., if file 1 has 3 lines, file 2 starts at line 4. Currently numbering restarts at 1 per file. You'd need to track a counter outside the for loop.
| function countHelper(inputFiles) { | ||
| return { | ||
| lines: inputFiles.split('\n').length - 1, | ||
| words: inputFiles.split(' ').filter(w => w !== "").length, |
There was a problem hiding this comment.
It means we split only according to space, but will ignore for example newline character or other whitespace characters. Could you fix it please?
| // .length; | ||
| // console.log(countOfWordsContainingEs); | ||
|
|
||
| function countHelper(inputFiles) { |
There was a problem hiding this comment.
countHelper is not very clear name, could you rename so naming reflects the purpose of the function?
SlideGauge
added
Reviewed
2 participants
Self checklist
Changelist
js tools.